Skip to content

FIX: missing ip settings from SDK package #390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 13, 2025
Merged

FIX: missing ip settings from SDK package #390

merged 3 commits into from
Aug 13, 2025
+148 −13

Conversation

lucas-zimerman
Copy link
Collaborator

Based on getsentry/sentry-javascript#17364

original context;

This PR fixes a long-standing problem in the SDK where it would set incorrect information about when Relay should (not) infer IP addresses from sent envelope requests.

Previously, this was thought to be controlled by setting event.user.ip_address: '{{auto}}'. However, after an incident in Relay, it was determined that this is in fact not a reliably way to control IP inference. Instead, SDKs should set event.sdk.settings.infer_ip: 'auto' | 'never' (see closes https://github.com/getsentry/sentry-javascript/issues/16252).
Unfortunately, this wasn't implemented immediately but is taken care of in this PR.

(FWIW, the only reason why Relay continued to infer IP addresses for the JS SDK was because it is excempt from logic that would infer IP addresses only if user.ip_address was set to '{{auto}}'. This is necessary to backwards compatibility with older SDKs.)

Follow-ups: We likely also need to adjust the logic in Electron and Lynx (at the very least remove setting user.ip_address).

closes https://github.com/getsentry/sentry-javascript/issues/17351
closes https://github.com/getsentry/sentry-javascript/issues/16252

Instead of bumping o V10, the fix was patched on the current release.

@lucas-zimerman
Copy link
Collaborator Author

Sample consumed events with default Pii on and off

https://us.sentry.io/api/0/projects/sentry-sdks/sentry-cordova/events/bdfbabca727a4ddcabd4630db56407a4/json/
https://us.sentry.io/api/0/projects/sentry-sdks/sentry-cordova/events/527440a19973400196d1388140cf39cb/json/

@lucas-zimerman lucas-zimerman marked this pull request as ready for review August 13, 2025 11:29
@lucas-zimerman lucas-zimerman requested a review from antonis August 13, 2025 11:29
seer-by-sentry[bot]
seer-by-sentry bot reviewed Aug 13, 2025
View reviewed changes
src/js/integrations/sdkinfo.ts Outdated
Comment on lines 29 to 35
let defaultPii: boolean | undefined = undefined;

const client = getClient();
if (client) {
const options = client?.getOptions();
defaultPii = options.sendDefaultPii;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug: The SdkInfo integration caches the sendDefaultPii option during initial setup. If Sentry.init() is called again, the integration uses a stale value, leading to incorrect IP inference.

  • Description: The SdkInfo integration captures the sendDefaultPii option value within the setupOnce method. Because setupOnce is designed to run only once per integration lifecycle, this value becomes stale if the Sentry SDK is re-initialized with different options via a subsequent Sentry.init() call. For example, if an application initializes with sendDefaultPii: false and later re-initializes with sendDefaultPii: true after obtaining user consent, the event processor will continue to use the initial false value. This results in infer_ip being incorrectly set to 'never', causing a loss of IP address data.

  • Suggested fix: To ensure the most current options are always used, move the client and option retrieval logic from setupOnce into the addEventProcessor callback. This will fetch getClient().getOptions() for each event, reflecting any changes from SDK re-initialization.
    severity: 0.65, confidence: 0.9

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't wrong but we don't guarantee mutability of SDK options and JS has the same behaviour
(happy to leave the call up to you if you wanna change it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK doesn't behave well from what I tested on multiple initializations, and since the SDK is on low maintenance I'd say it's good enough.

@kahest kahest requested a review from Lms24 August 13, 2025 11:56
Lms24
Lms24 approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Same minor concern as in capacitor but LGTM!

src/js/integrations/sdkinfo.ts Outdated
Comment on lines 29 to 35
let defaultPii: boolean | undefined = undefined;

const client = getClient();
if (client) {
const options = client?.getOptions();
defaultPii = options.sendDefaultPii;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't wrong but we don't guarantee mutability of SDK options and JS has the same behaviour
(happy to leave the call up to you if you wanna change it)

antonis
antonis approved these changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@antonis antonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎸

@lucas-zimerman
Copy link
Collaborator Author

Checked that user ip wasn't being set when sendDefaultPii was set to auto

@lucas-zimerman lucas-zimerman merged commit 7e68e24 into main Aug 13, 2025
11 checks passed
@lucas-zimerman lucas-zimerman deleted the lz/fiz-auto-ip branch August 13, 2025 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seer-by-sentry seer-by-sentry[bot] seer-by-sentry[bot] left review comments

@antonis antonis antonis approved these changes

@Lms24 Lms24 Lms24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucas-zimerman @antonis @Lms24